Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add optional namespace arguments for make_class #1203

Merged
merged 8 commits into from
Nov 28, 2023
Merged

Add optional namespace arguments for make_class #1203

merged 8 commits into from
Nov 28, 2023

Conversation

aarnphm
Copy link
Contributor

@aarnphm aarnphm commented Nov 13, 2023 •
edited
Loading

Signed-off-by: Aaron 29749331+aarnphm@users.noreply.github.com

Summary

Add an optional namespaces argument to attr.make_class to allow users to inject addtional namespaces attributes into newly created attrs class.

I think this is pretty useful for cases where a derivation of the base class is needed with additional functionalities. Since make_class use types.new_class under the hood this is possible.

Currently, if users want to derivate from a attrs classes, one might do:

import attr

@attr.define
class Base:
    def a(self): ...

def b(self: type[Base], something_else: int) -> int: return something_else + 1

new_class = types.new_class("NewClass", (Base,), exec_body=lambda ns: ns.update({"b": b}))

This will lose types information and therefore users might need to provide additional stubs for the new class. With this PR, users can do:

import attrs

@attr.define
class Base:
    def a(self): ...

def b(self: type[Base], something_else: int) -> int: return something_else + 1

new_class = attr.make_class("NewClass", bases=(Base,), namespaces={"b": b})

Which is nice, but might not be a good idea.

Pull Request Check List

  • Do not open pull requests from your main branch – use a separate branch!
    • There's a ton of footguns waiting if you don't heed this warning. You can still go back to your project, create a branch from your main branch, push it, and open the pull request from the new branch.
    • This is not a pre-requisite for your your pull request to be accepted, but you have been warned.
  • Added tests for changed code.
    Our CI fails if coverage is not 100%.
  • New features have been added to our Hypothesis testing strategy.
  • Changes or additions to public APIs are reflected in our type stubs (files ending in .pyi).
    • ...and used in the stub test file tests/typing_example.py.
    • If they've been added to attr/__init__.pyi, they've also been re-imported in attrs/__init__.pyi.
  • Updated documentation for changed code.
    • New functions/classes have to be added to docs/api.rst by hand.
    • Changes to the signature of @attr.s() have to be added by hand too.
    • Changed/added classes/methods/functions have appropriate versionadded, versionchanged, or deprecated directives.
      The next version is the second number in the current release + 1.
      The first number represents the current year.
      So if the current version on PyPI is 22.2.0, the next version is gonna be 22.3.0.
      If the next version is the first in the new year, it'll be 23.1.0.
  • Documentation in .rst and .md files is written using semantic newlines.
  • Changes (and possible deprecations) have news fragments in changelog.d.
  • Consider granting push permissions to the PR branch, so maintainers can fix minor issues themselves without pestering you.

@aarnphm
feat(make_class): add optional namespace arguments to pass through to
d9119e5 
`make_class`

Signed-off-by: Aaron <29749331+aarnphm@users.noreply.github.com>
@aarnphm
chore: add changelog
8332539 
Signed-off-by: Aaron <29749331+aarnphm@users.noreply.github.com>
@aarnphm aarnphm changed the title feat(make_class): add optional namespace arguments to pass through to make_class feat: add optional namespace arguments to pass through to make_class Nov 13, 2023
@aarnphm aarnphm changed the title feat: add optional namespace arguments to pass through to make_class Add optional namespace arguments to pass through to make_class Nov 13, 2023
@aarnphm aarnphm changed the title Add optional namespace arguments to pass through to make_class Add optional namespace arguments for make_class Nov 13, 2023
@hynek
Copy link
Member

hynek commented Nov 17, 2023

I think this is an interesting idea as per our ideal to be a class construction kit!

But I don't think namespaces is a good term for what it does. Wanna brainstorm something more obvious? Maybe class_body?

@aarnphm
Copy link
Contributor Author

aarnphm commented Nov 17, 2023 •
edited
Loading

I think this is an interesting idea as per our ideal to be a class construction kit!

But I don't think namespaces is a good term for what it does. Wanna brainstorm something more obvious? Maybe class_body?

I think class_body makes sense. My guess that from a Python developer perspective, this would infer that attributes here are class attributes.

Though I'm not quite sure if users implement a __class_getitem__ override, how would that work?

__class_getitem__ would probably have access to all transpiled attributes instead of the _CountingAttr?

Oh I forgot to mention that for type inference, mypy plugin for attr.make_class will need to be implemented as well, otherwise B.echo will raise a type error.

@aarnphm
Rename namespaces to class_body
984a592 
Signed-off-by: Aaron <29749331+aarnphm@users.noreply.github.com>
@aarnphm
Fix versionchanged for correct features
e4ebeb3 
Signed-off-by: Aaron <29749331+aarnphm@users.noreply.github.com>
@aarnphm
Correctly name test case
9c8c954 
Signed-off-by: Aaron <29749331+aarnphm@users.noreply.github.com>
@aarnphm
Update changelog
6e67abe 
Signed-off-by: Aaron <29749331+aarnphm@users.noreply.github.com>
@hynek
Copy link
Member

hynek commented Nov 18, 2023

I think this is an interesting idea as per our ideal to be a class construction kit!
But I don't think namespaces is a good term for what it does. Wanna brainstorm something more obvious? Maybe class_body?

I think class_body makes sense. My guess that from a Python developer perspective, this would infer that attributes here are class attributes.

I mean… they are? Am I misunderstanding what your code is doing? 😅

Though I'm not quite sure if users implement a __class_getitem__ override, how would that work?

Can you elaborate what you mean by that please?

__class_getitem__ would probably have access to all transpiled attributes instead of the _CountingAttr?

Oh I forgot to mention that for type inference, mypy plugin for attr.make_class will need to be implemented as well, otherwise B.echo will raise a type error.

That sounds like a @Tinche problem. 😇

@aarnphm
Copy link
Contributor Author

aarnphm commented Nov 18, 2023 •
edited
Loading

I think class_body makes sense. My guess that from a Python developer perspective, this would infer that attributes here are class attributes.

I mean... they are? Am I misunderstanding what your code is doing? 😅

Oh, I think we are aligned on this.

Though I'm not quite sure if users implement a __class_getitem__ override, how would that work?

Can you elaborate what you mean by that please?

Yes, so for the following example

import attr

@attr.define
class Base:
    x: int
    y: t.List[int] = attr.field()

def __class_getitem_override(cls, item): # what is the type of item here?
    if item.__name__ == 'y': return [1,2,3]
    else: return cls.__getitem__(item)

new_class = attr.make_class("NewClass", bases=(Base,), namespaces={"__class_getitem__": __class_getitem_override})

Users cannot be prevented from doing something like this, right? What should be the type of item here? In true PEP560 fashion, this should be a class attribute, which means it is the _CountingAttr type here instead of attr.Attribute.

@hynek
Copy link
Member

hynek commented Nov 18, 2023

Oh god TIL about __class_getitem__. Uhh… Tiiiiiin?

@aarnphm
Copy link
Contributor Author

aarnphm commented Nov 18, 2023

Oh god TIL about __class_getitem__. Uhh… Tiiiiiin?

This is the one concern I have when thinking about this feature, so probably will def need a discussion about this.

@hynek
Copy link
Member

hynek commented Nov 28, 2023

OK since this seems to be a somewhat stalling issue: is your PR in any way special to __class_getitem__?

From reading the docs around it, I understand it's a fallback if there's no __getitem__ and if the user implemented it. But, that doesn't seem to be our business? We don't implement that method, nor do we deal with __getitem__ – so the user can do whatever?

Like we try hard to not create attractive nuisances, but we also try to not be too limiting.

@aarnphm
Copy link
Contributor Author

aarnphm commented Nov 28, 2023

Ye I think it should be fine. So that is probably not an issue. This one is ready to merge.

hynek
hynek approved these changes Nov 28, 2023
View reviewed changes
Copy link
Member

@hynek hynek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@hynek hynek added this pull request to the merge queue Nov 28, 2023
Merged via the queue into python-attrs:main with commit 1fcd29f Nov 28, 2023
20 checks passed
@aarnphm aarnphm deleted the feat/make-class-type branch November 28, 2023 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hynek hynek hynek approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@aarnphm @hynek